Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make webhooks more resilient #2516

Closed
wants to merge 1 commit into from

Conversation

Lyncredible
Copy link
Contributor

@Lyncredible Lyncredible commented Oct 16, 2023

  • Do not quit on subscription API errors. Instead, wait a configurable interval before retrying.
  • Shorten default webhook expiration and renewal intervals to reduce the chance of http 409 errors.
  • Handle http 409 on subscription creation by taking over the existing subscription.
  • Handle http 404 on subscription renewal by creating a new subscription(current behavior, listed for completeness).
  • Log other known http errors include 400 (bad webhook endpoint), 401 (auth failed), and 403 (too many subscriptions).
  • Log detailed messages when encoutering unknown http errors to assist with future debugging.

Fixes #2501

@Lyncredible
Copy link
Contributor Author

@abraunegg It is probably easier to review this with ?w=1 appended to the URL to ignore whitespace changes. My editor does not like trailing whitespaces as usual :-P

@abraunegg
Copy link
Owner

@Lyncredible
Thanks for your PR, however I am not going to add this into v2.4.

Webhooks is still missing from v2.5 branch, due to be added back in shortly, so what I will do, once those changes are added back in, circle back and either cherry pick your changes or ask you to re-work your PR to align to v2.5

@abraunegg abraunegg added the On Hold On Hold label Oct 16, 2023
@Lyncredible
Copy link
Contributor Author

@abraunegg Sounds good. Take your time.

My local box is already running on this PR (on top of 4a60654 in master). It has been working well, but let me know if you want me to also test this on a different branch/commit.

* Do not quit on subscription API errors. Instead, wait a configurable interval before retrying.
* Shorten default webhook expiration and renewal intervals to reduce the chance of http 409 errors.
* Handle http 409 on subscription creation by taking over the existing subscription.
* Handle http 404 on subscription renewal by creating a new subscription(current behavior, listed for completeness).
* Log other known http errors include 400 (bad webhook endpoint), 401 (auth failed), and 403 (too many subscriptions).
* Log detailed messages when encoutering unknown http errors to assist with future debugging.
@abraunegg
Copy link
Owner

@Lyncredible

The 'alpha-2' PR (#2505) is just about 'feature parity complete' - with the exception of adding back in webhooks.

I am going to start doing that tomorrow or the next day as part of 'alpha-3' - but it would be great once that lands if you can review and/or test to ensure that things are working correctly.

@abraunegg
Copy link
Owner

@Lyncredible
Please can you review comments in #2520

abraunegg added a commit that referenced this pull request Oct 19, 2023
* Update webhook feature with #2516 changes
@abraunegg
Copy link
Owner

@Lyncredible
Please can you validate 'alpha-4' and if this is all OK, close this PR ?

@Lyncredible
Copy link
Contributor Author

@abraunegg It has been working fine for the last ~12 hours. Closing this PR. Thanks for incorporating the fix!

@abraunegg
Copy link
Owner

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
On Hold On Hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Subscription ID already exists
2 participants